Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH Add Pyodide support #7803

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

hoodmane
Copy link

@hoodmane hoodmane commented Nov 8, 2023

What do these changes do?

Adds support for Pyodide. Intended for discussion. Probably not an implementation we'd want to merge. It seems to be good enough for several downstream projects I've tested out.

I will try to get some simple CI working on this branch soon.

Related issue number

#7253

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@Dreamsorcerer
Copy link
Member

Do you think we could encapsulate that logic into a custom Connector in some way, so _request() can work with minimal changes?

@hoodmane
Copy link
Author

I think so. At the very least we'll need _request to check if it's Emscripten and probably wrap the Connection object into a PyodideConnection and use a PyodideRequest, but I think if we add:

if sys.platform == "emscripten":
   connector = PyodideConnector(connector)
   request_class = PyodideRequestClass(request_class)

in ClientSession.__init__ and provide appropriate implementations of PyodideConnector and PyodideRequestClass then it will work.

@Dreamsorcerer
Copy link
Member

Yes, I think specifically, connector defaults to None. So, we can adjust the code to use the new connector on this platform or TCPConnector otherwise. So far, looks reasonable if you'd like to continue with this and complete the feature.

@psymbio
Copy link

psymbio commented Nov 16, 2023

Currently, for this issue, I compiled a wheel for multidict v4.7.6 (needs to be v5.0.0 <= and v4.5.0); and I try to install it inside jupyterlite and get the following issue:

Code for recreation:

import micropip
await micropip.install('https://raw.githubusercontent.com/psymbio/tiktoken_rust_wasm/main/packages/multidict/multidict-4.7.6-py3-none-any.whl', keep_going=True)
await micropip.install("aiohttp", verbose=True)
import aiohttp
Error: ```python
AttributeError                            Traceback (most recent call last)
Cell In[2], line 1
----> 1 import aiohttp

File /lib/python3.11/site-packages/aiohttp/__init__.py:6
      3 from typing import Tuple  # noqa
      5 from . import hdrs as hdrs
----> 6 from .client import BaseConnector as BaseConnector
      7 from .client import ClientConnectionError as ClientConnectionError
      8 from .client import (
      9     ClientConnectorCertificateError as ClientConnectorCertificateError,
     10 )

File /lib/python3.11/site-packages/aiohttp/client.py:32
     29 from multidict import CIMultiDict, MultiDict, MultiDictProxy, istr
     30 from yarl import URL
---> 32 from . import hdrs, http, payload
     33 from .abc import AbstractCookieJar
     34 from .client_exceptions import ClientConnectionError as ClientConnectionError

File /lib/python3.11/site-packages/aiohttp/http.py:7
      5 from . import __version__
      6 from .http_exceptions import HttpProcessingError as HttpProcessingError
----> 7 from .http_parser import HeadersParser as HeadersParser
      8 from .http_parser import HttpParser as HttpParser
      9 from .http_parser import HttpRequestParser as HttpRequestParser

File /lib/python3.11/site-packages/aiohttp/http_parser.py:15
     13 from . import hdrs
     14 from .base_protocol import BaseProtocol
---> 15 from .helpers import NO_EXTENSIONS, BaseTimerContext
     16 from .http_exceptions import (
     17     BadStatusLine,
     18     ContentEncodingError,
   (...)
     22     TransferEncodingError,
     23 )
     24 from .http_writer import HttpVersion, HttpVersion10

File /lib/python3.11/site-packages/aiohttp/helpers.py:100
     96 TOKEN = CHAR ^ CTL ^ SEPARATORS
     99 coroutines = asyncio.coroutines
--> 100 old_debug = coroutines._DEBUG  # type: ignore
    102 # prevent "coroutine noop was never awaited" warning.
    103 coroutines._DEBUG = False  # type: ignore

AttributeError: module 'asyncio.coroutines' has no attribute '_DEBUG'
```

aiohttp is trying to access asyncio.coroutines._DEBUG - which doesn't exist.

@Dreamsorcerer
Copy link
Member

aiohttp is trying to access asyncio.coroutines._DEBUG - which doesn't exist.

Except, that's not our code. No idea what is happening with your traceback there, but that code doesn't exist in master, 3.8 or 3.9:
https://github.com/aio-libs/aiohttp/blob/3.8/aiohttp/helpers.py#L100

@psymbio
Copy link

psymbio commented Nov 16, 2023

Sorry, I forgot to mention the fact that this was done in JupyterLite, so according to this comment (pyodide/pyodide#4070 (comment)) the version installed was 3.6.2 (currently I'm not sure but after just glancing through the releases I can see v3.6.2 is probably the last release with a pure python wheel) so this issue exists there.

I think I'll try building a pure Python wheel for v3.8.6 and update the issue here.

Also, can building the pure Python wheel be added to the process?

@Dreamsorcerer
Copy link
Member

Also, can building the pure Python wheel be added to the process?

If you create a PR, then probably. See #7632 too.
Although I don't really understand what a pure Python wheel offers that an sdist doesn't...

hoodmane added a commit to pyodide/pyodide that referenced this pull request Nov 18, 2023
This doesn't actually work without an additional patch but it's possible
to get it to work by monkey patching a method. 

See discussion about adding support for Pyodide to aiohttp:
aio-libs/aiohttp#7253
aio-libs/aiohttp#7803
@hoodmane hoodmane changed the title WIP Demo of Pyodide support ENH Add Pyodide support Nov 20, 2023
@hoodmane
Copy link
Author

@Dreamsorcerer could you approve the CI to run? Ad hoc local tests run but I'd like to get a basic test running in CI.

aiohttp/client_reqrep.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 59 lines in your changes are missing coverage. Please review.

Comparison is base (43f92fa) 97.36% compared to head (2607d49) 97.25%.

Files Patch % Lines
aiohttp/client_reqrep.py 20.00% 40 Missing ⚠️
aiohttp/connector.py 43.33% 17 Missing ⚠️
aiohttp/helpers.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7803      +/-   ##
==========================================
- Coverage   97.36%   97.25%   -0.11%     
==========================================
  Files         107      107              
  Lines       32346    32440      +94     
  Branches     3748     3758      +10     
==========================================
+ Hits        31494    31551      +57     
- Misses        647      688      +41     
+ Partials      205      201       -4     
Flag Coverage Δ
CI-GHA 97.17% <39.79%> (-0.12%) ⬇️
OS-Linux 96.84% <39.79%> (-0.12%) ⬇️
OS-Windows 95.34% <35.86%> (-0.18%) ⬇️
OS-macOS 96.66% <39.79%> (+<0.01%) ⬆️
Py-3.10.11 95.26% <35.86%> (-0.18%) ⬇️
Py-3.10.13 96.65% <39.79%> (-0.18%) ⬇️
Py-3.11.6 96.36% <39.79%> (-0.18%) ⬇️
Py-3.12.0 96.46% <39.79%> (-0.15%) ⬇️
Py-3.8.10 95.23% <35.86%> (-0.18%) ⬇️
Py-3.8.18 96.59% <39.79%> (-0.18%) ⬇️
Py-3.9.13 95.23% <35.86%> (-0.18%) ⬇️
Py-3.9.18 96.62% <39.79%> (-0.18%) ⬇️
Py-pypy7.3.13 96.17% <39.79%> (?)
VM-macos 96.66% <39.79%> (+<0.01%) ⬆️
VM-ubuntu 96.84% <39.79%> (-0.12%) ⬇️
VM-windows 95.34% <35.86%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoodmane
Copy link
Author

@Dreamsorcerer I guess the workflows need to be separately approved for each commit?


class JsArrayBuffer(Protocol):
def to_bytes(self) -> bytes:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
headers: List[Tuple[str, str]]

def arrayBuffer(self) -> asyncio.Future[JsArrayBuffer]:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
@hoodmane
Copy link
Author

@Dreamsorcerer please trigger CI when you get a chance

@Dreamsorcerer
Copy link
Member

I'm around most of tomorrow (and the next few hours) if you need me to keep approving.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 5, 2023
@juntyr
Copy link

juntyr commented Jul 22, 2024

Thanks for your work! Has there been any progress on this?

@Dreamsorcerer Dreamsorcerer added the pr-unfinished The PR is unfinished and may need a volunteer to complete it label Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR pr-unfinished The PR is unfinished and may need a volunteer to complete it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants